-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: 支持CompositeCommand间组合 #2125
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
请执行 ./gradlew :mirai-console:apiDumpAll
并提交变更的文件
看了下英文下描述 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果使用 ChildCommand
的话, 她给我的第一印象是
object TopLevel : CompositeCommand("toplevel") {
val myCommand = object : CompositeCommand() {
@SubCommand
suspend fun CommandSender.handle() {}
}
}
命令会按照 /toplevel myCommand handle
注册, 我个人感觉这个名字会有很大的误解性.
也许可以考虑 @CombinedCommand
或者 @MixinedCommand
? @Him188
@Karlatemp 我也是这样想的, 今天忙, 看了没来得及写 review 我认为造一个其他概念而不要用 |
另外现在这样, |
不过单看“XX助手”场景,有的功能点可能由SimpleCommand提供,所以需要的是能混合组合SimpleCommand和CompositeCommand 的工具。从这个角度来看, |
我考虑了 hierarchy 的,
|
看了下代码, 另外我还是不太理解你说的兼容性和复用性指什么,能更具体的说明吗? |
@hundun000 是的 兼容性指的是有没有可能让开发者用 |
我的理解是要让
是指变成这样吗,那不就又没区别了?
|
|
懂了 |
20a8b8a
to
df170b9
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
总体来说是很不错的实现, 有一些考虑:
SubCommandGroup
最好要提供一个可选的名称前缀的功能, 就像CompositeCommand
那样. 我建议在使用注解时- 在实现上一点后,
CompositeCommand
实际上也是一个SubCommandGroup
. 把SubCommandGroup
的provideOverloads
改为overloads
,CompositeCommand
和AbstractSubCommandGroup
就可以都继承SubCommandGroup
. 这解决了各种类型兼容问题, 但这将会允许CompositeCommand
和SubCommandGroup
互相嵌套. 嵌套方案在之后讨论 - 互相嵌套时
CommandReflector
会不会报错? 需要增加测试 - 需要增加对
generateUsage
的测试
有关我上面说的设计方案, 由于 console 的指令是定义式的, 我想象了如下使用 group 的方式, 欢迎评论:
// 插件一个模块的部分功能
class ModuleAPart1 : AbstractSubCommandGroup() {
@SubCommand
fun function1();
}
// 插件一个模块的另一部分功能
class ModuleAPart2 : AbstractSubCommandGroup() {
@SubCommand
fun function2();
@SubCommand
fun function3();
}
// 插件的一个模块, 统合上面两个功能
class ModuleACommandGroup: AbstractSubCommandGroup() {
@SubCommand // 与在函数上标注这个注解类似, 它会带 `part1` 这个名称前缀来注册指令. 需要执行 /base part1 function1, 也可以使用 SubCommand 的参数来覆盖名称
val part1: ModuleAPart1
@FlattenSubCommands // 新增, 不带前缀注册指令, 执行 /base function2
val part1: ModuleAPart1
}
// 插件的总指令, 统合全部模块
class MyUnifiedCommand : CompositeCommand() {
@SubCommand
val moduleA: ModuleACommandGroup
@SubCommand // 与现有注册方式混合, 看起来也不错
fun about();
}
注解方案
新设计的 CompositeCommand
与 SubCommandGroup
在定义指令上是一样的, 我希望让它们使用同样的注解. 我觉得可以把那些 @SubCommand
复制到新抽象的 SubCommandGroup
中并 public. 将旧的标注 @Deprecated(level = HIDDEN)
. 这样 Kotlin 用户只需要修复 import (因为 Kotlin 引用非直接父类的内部类需要带外部类类名), Java 用户无需更新.
嵌套方案
嵌套是否需要增加前缀, 在且只在外层定义中指定.
- 使用
@SubCommand
时, 增加前缀. 前缀的名称的行为与这个注解标注在指令上时一致——有参数使用参数, 无参数使用指令反射名称 - 使用新增的
@FlattenSubCommands
(这是我建议的名称, 可考虑更好名称) 时, 不增加前缀. 这样的行为类似标准库集合的flatMap
.
有关混合嵌套问题, 我考虑:
- 允许
AbstractSubCommandGroup
嵌入CompositeCommand
——原 PR 计划的功能 - 允许
CompositeCommand
嵌入AbstractSubCommandGroup
, 即使这可能导致 confusion, 但这毕竟是用户自己要这么干的. 以后可考虑通过 IDE 插件提供预览来解决这个问题 (指令增加 group 之后变得复杂, 也确实可能需要预览了)
有关用户自己实现接口的考虑:
接口公开, 且容易实现. 我认为可以允许用户实现, 只要符合接口的设计规范.
mirai-console/backend/mirai-console/src/command/SubCommandGroup.kt
Outdated
Show resolved
Hide resolved
mirai-console/backend/mirai-console/src/internal/command/CommandReflector.kt
Outdated
Show resolved
Hide resolved
public constructor( | ||
ownerCommand: Command, | ||
owner: Any, | ||
correspondingFunction: KFunction<*>, | ||
message: String?, | ||
) : super("Illegal command declaration: ${correspondingFunction.name} declared in ${ownerCommand::class.qualifiedName}") { | ||
) : super("Illegal command declaration: ${correspondingFunction.name} declared in ${owner::class.qualifiedName}") { | ||
this.message = message | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
增加一个适用于 CommandGroup 的构造器, 而不是修改参数为 Any
即使已经(level = HIDDEN)且 计划开发期间真实移除 类似问题的issue已被记录为kotlin bug。 |
真实移除是 ABI 变更,是不可能进行的,只能考虑其他方案了... |
@hundun000 @Karlatemp 有什么想法吗,关于新注解 |
x Kotlin 用户只需要修复 import 这样?因为我看IDEA自动补全也是选择补全为这样(指以前CompositeCommand.SubCommand还未废弃的时候),是因为这样也更符合编码规范吗? |
这个就是 Kotlin 设计问题,不允许用 simple name 引用超过一级继承关系的内部类型。Java 是可以的 |
…p.kt Co-authored-by: Him188 <[email protected]>
…ndReflector.kt Co-authored-by: Him188 <[email protected]>
9916ce2
to
8b30851
Compare
效果:
@ChildCommad
注解的CompositeCommand子类成员中的所有子指令,也会成为他的子指令。(见InstanceTestCommand.kt变动)改动说明:
@ChildCommad
。也可以考虑继续使用@SubCommad
?以及child的命名可能会引起歧义想到继承关系